#47037 - Create Remote Dialer Proxy #90
Conversation
* Add a port-forward type * Add support for automatic reconnects when pods are terminated * List clients * Add a Dockerfile for the proxy * Add proxy * Update go.mod * Update package name * Add config parsing * Immediately close connection when there are no clients
gehrkefc
changed the title
|
|
||
| if failed { | ||
| return err | ||
| } |
There was a problem hiding this comment.
We can probably get rid of failed and use err in the same way
var readyErr error
...
if err := r.runForwarder(ctx, r.readyCh, r.stopCh, r.Ports); err != nil {
if errors.Is(err, portforward.ErrLostConnectionToPod) {
logrus.Errorf("Lost connection to pod (no automatic retry in this refactor): %v", err)
} else {
logrus.Errorf("Non-restartable error: %v", err)
readyErr = err
r.readyCh <- struct{}{}
return
}
}
...
if readyErr != nil {
return err
}
There was a problem hiding this comment.
Sure, just changed it. Thanks!
There was a problem hiding this comment.
I think this will trigger a race condition with the race detector because we have:
G1: write readyErr (init)
G2: write readyErr
G1: read readyErr
There was a problem hiding this comment.
Unresolving this due to my comment above^ Do you disagree @joshmeranda / @gehrkefc
|
@gehrkefc Currently this PR points to |
|
|
||
| if failed { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I think this will trigger a race condition with the race detector because we have:
G1: write readyErr (init)
G2: write readyErr
G1: read readyErr
forward/forward.go
Outdated
| logrus.Infoln("Goroutine stopped.") | ||
| return | ||
| default: | ||
| err := r.runForwarder(ctx, r.readyCh, r.stopCh, r.Ports) |
There was a problem hiding this comment.
Could we replace r.stopCh with ctx.Done() ?
proxyclient/client.go
Outdated
| secret, err := secretController.Get(namespace, certSecretName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| crtData, exists := secret.Data["tls.crt"] | ||
| if !exists { | ||
| return nil, fmt.Errorf("secret %s/%s missing tls.crt field", namespace, certSecretName) | ||
| } | ||
|
|
||
| rootCAs := x509.NewCertPool() | ||
| if ok := rootCAs.AppendCertsFromPEM(crtData); !ok { | ||
| return nil, fmt.Errorf("failed to parse tls.crt from secret into a CA pool") | ||
| } |
There was a problem hiding this comment.
The certs will be rotated by dynamiclistener before they expire (or at least that's the intention) so we should react to changes for this. We can add that functionality in the next PR, same as we're doing with @joshmeranda 's PR. We do need that though as part of the GH issue.
There was a problem hiding this comment.
I've implemented this. Please check it. I tested it modifying a local copy of dynamic listener to generate a new cert at each 20s. Please check the image below to see both working.
In the lower terminal you'll see the server regenerating the secret and updating it. Then in the upper terminal you'll see the client receiving the event with the new cert.
I've made another change in the server in the runProxyListener function because of a high cpu usage. |
| core, err := core.NewFactoryFromConfigWithOptions(restConfig, nil) | ||
| if err != nil { | ||
| return fmt.Errorf("build secret controller failed: %w", err) | ||
| } |
There was a problem hiding this comment.
We typically try not to create too many controller factories in Rancher to avoid duplicating data (memory). Since this client is going to run in Rancher, and Rancher already has controllers / caches for secrets, let's reuse those. To do this, you can make it so that New() takes a SecretController, and when we integrate with Rancher, we'll pass the controller.
| secretController.Informer().AddEventHandler( | ||
| cache.ResourceEventHandlerFuncs{ | ||
| UpdateFunc: func(oldSecret, newSecret interface{}) { |
There was a problem hiding this comment.
| secretController.Informer().AddEventHandler( | |
| cache.ResourceEventHandlerFuncs{ | |
| UpdateFunc: func(oldSecret, newSecret interface{}) { | |
| secretController.OnChange(ctx, "remotedialer-proxy", func(_ string, secret *corev1.Secret) (*corev1.Secret, error) { |
OnChange is the most typical way of adding a controller. Informer().AddEventHandler() is more low-level (and I haven't really seen it used, but it might because I wasn't looking at places that use it)
| UpdateFunc: func(oldSecret, newSecret interface{}) { | ||
| updatedSecretCert, ok := newSecret.(*corev1.Secret) | ||
| if ok { | ||
| if updatedSecretCert.Name == c.certSecretName { |
There was a problem hiding this comment.
Both namespace and name should be checked.
| if err := core.Start(ctx, 1); err != nil { | ||
| return fmt.Errorf("secret controller factory start failed: %w", err) | ||
| } |
There was a problem hiding this comment.
This can be removed since we're just going to pass in the controller, the controller will already be started in Rancher.
Issue: rancher/rancher#47037